fix(agents): recover empty final responses once - #198
fix(agents): recover empty final responses once#198jasonqlwilliams-alt wants to merge 13 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6883c80514
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ...(resumeSessionId && !userSpecifiedSessionContinuation(userArgs) | ||
| ? ["--resume", resumeSessionId] | ||
| : []), |
There was a problem hiding this comment.
When agentArgsOverride.claude contains -c or --continue, this condition suppresses --resume <captured-session-id> during the empty-response recovery, while the original broad continuation flag remains in userArgs. If another Claude invocation becomes the most recent session between turns, the nudge can enter an unrelated conversation and return a fabricated summary; either override the continuation flag with the captured ID or skip recovery for this configuration. This also contradicts the documented guarantee that Claude resumes the exact session.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
…oss session agents
…dle gate and usage
…og stream lifetime
…e; complete codex denylist
…l and failed continuations
6883c80 to
f024004
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 74cdae9790
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| setupChildProcessHandlers(child, "copilot", logStream, reject, () => { | ||
| setupChildProcessHandlers(child, "copilot", reject, () => { | ||
| if (!lastAgentMessage) { |
There was a problem hiding this comment.
When Copilot emits an assistant.message whose data.content contains only spaces or newlines, lastAgentMessage is truthy, so this guard falls through to parseAgentOutput and reports a generic JSON parse failure instead of COPILOT_EMPTY_RESPONSE_MESSAGE. Copilot should remain non-retryable, but the newly documented failure contract says a skipped nudge names the unverified-resume reason, so trim the message when performing this emptiness check.
AGENTS.md reference: AGENTS.md:L10-L10
Useful? React with 👍 / 👎.
1 participant
Intent
Resolve issue #163 with the preserved committed implementation. When an agent turn otherwise completes but its final response is empty, send exactly one continuation nudge before recording the iteration as failed. If that one retry is also empty, fail normally; never loop or add another retry. Make the continuation nudge visible in the run log. Exclude GitHub Copilot from recovery because its resume contract is unverified. Exclude Pi and document the exclusion because Pi has no recoverable context under --no-session. Do not broaden session behavior, add new native adapters, or change unrelated recovery contracts. Revalidate and deliver the committed work through the existing standard no-mistakes contribution path and PR #198; maintainers alone may merge.
What Changed
Risk Assessment
🚨 High: The main recovery flow is well bounded, but a supported Claude configuration still violates the authoritative exact-session and no-session-broadening requirements, requiring explicit approval or correction before merge.
Testing
After correcting preliminary test-only expectations to the actual persisted output contracts, the focused adapter tests, production build, normal CLI path, successful recovery, and exhausted single-retry path all passed. Manual CLI evidence confirms one visible same-session nudge, no third attempt, correct success/failure recording, and expected git outcomes; this non-UI change is demonstrated with transcripts and persisted logs rather than screenshots.
Evidence: Recovered CLI run
Successful run: 1 good iteration and 1 commit.Evidence: Successful agent turns
Exactly two turns used session-1; the second contained only the continuation nudge.Evidence: Successful recovery events
One continuation event followed by a successful iteration.Evidence: Exhausted CLI run
Exhausted run: 1 failed iteration and 0 commits.Evidence: Exhausted agent turns
Exactly two empty turns used session-1, proving no third retry occurred.Evidence: Exhausted recovery events
One continuation event followed by the normal empty-response failure.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/core/agents/claude.ts:188- WhenagentArgsOverride.claudecontains-cor--continue, the retry keeps that recency-based flag and suppresses--resume <captured-session-id>. Another Claude invocation can therefore become “most recent,” causing the nudge to enter an unrelated conversation and fabricate the iteration summary; an ID-less turn is also incorrectly considered recoverable in this configuration. For continuation turns, require the captured session ID and replace the broad flag with--resume <id>; otherwise skip recovery.src/core/agents/acp.ts:488- The zero-length check treats whitespace-only ACP output as a final response. A completed turn emitting only spaces or newlines therefore reaches the JSON parse error instead of receiving the required one-shot continuation. Trim the output candidates when deciding whether the response is empty.src/core/agents/codex.ts:351- A whitespace-only Codexagent_messageis truthy, so a completed turn with blank text bypasses empty-response recovery and fails JSON parsing. Normalize withtrim()before the empty check so blank completed turns receive the one allowed nudge.🔧 Fix: fix exact-session and whitespace empty-response recovery
3 issues (1 error, 2 warnings) still open:
src/core/agents/claude.ts:172-buildClaudeArgsremoves-c/--continuefor recovery but preserves--fork-session. With the supported override--continue --fork-session, the retry adds--resume <captured-id>while retaining--fork-session, so Claude creates another session instead of resuming the exact one. This contradicts the required “Do not broaden session behavior” criterion and README’s exact-session claim. Decide whether recovery must strip--fork-sessionor whether this exception is intentional and should be documented.src/core/agents/codex.ts:89- The unsupported-argument check misses compact short-option values accepted bycodex exec, such as-C/tmp,-sworkspace-write, and-pwork.codex exec resumerejects these, so an otherwise recoverable empty turn launches a continuation that fails before delivering the nudge. Detect attached values for the denylisted short options.src/core/agents/copilot.ts:313- A whitespace-only Copilotassistant.messageis truthy, so it bypasses the documented empty-response exclusion and reports a generic JSON parse failure instead of naming the unverified resume contract. Trim when checking for an empty final message; recovery should remain disabled.✅ **Test** - passed
✅ No issues found.
Inspected the target diff and authoritative intent, including the documented Pi and Copilot exclusions.pnpm exec vitest run src/core/agents/empty-response.test.ts src/core/agents/acp.test.ts src/core/agents/claude.test.ts src/core/agents/codex.test.ts src/core/agents/copilot.test.ts src/core/agents/opencode.test.ts src/core/agents/pi.test.ts src/core/agents/rovodev.test.ts src/core/agents/stream-utils.test.ts src/core/config.test.tspnpm run buildAdded executable E2E coverage ine2e/e2e.test.tsand the mock OpenCode fixture.pnpm exec vitest run e2e/e2e.test.ts -t "runs one iteration from an argv prompt and cleans up the mock opencode server|recovers one completed empty turn in the same session and records the nudge|fails after one continuation when both completed turns are empty"Manually ran the built CLI withGNHF_MOCK_OPENCODE_EMPTY_ONCE=1and inspected its stdout summary, agent turns, commit,gnhf.log, andnotes.md.Manually ran the built CLI withGNHF_MOCK_OPENCODE_ALWAYS_EMPTY=1and verified two same-session turns, one logged nudge, normal failure, and no commit.Removed transient fixture repositories and verified only the intentional E2E test changes remain in the worktree.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.